Skip to content

feat: consolidate to llguidance from xgrammar #1077

Open
jakelorocco wants to merge 3 commits into
generative-computing:mainfrom
jakelorocco:fix/consolidate-llguidance
Open

feat: consolidate to llguidance from xgrammar #1077
jakelorocco wants to merge 3 commits into
generative-computing:mainfrom
jakelorocco:fix/consolidate-llguidance

Conversation

@jakelorocco
Copy link
Copy Markdown
Contributor

@jakelorocco jakelorocco commented May 14, 2026

Misc PR

Type of PR

  • Bug Fix
  • New Feature
  • Documentation
  • Other

Description

Moves granite formatters onto llguidance. There wasn't a strong reason to keep xgrammar and both work. All intrinsic tests continue to pass.

Moved _GuidanceLogitsProcessor to util so that it could be imported by the HuggingFace backend and not live in two spots.

Also fixed one spot in the util function where model was being used even though it could've been None.

Testing

  • Tests added to the respective file if code was changed
  • New code has 100% coverage if code as added
  • Ensure existing tests and github automation passes (a maintainer will kick off the github automation when the rest of the PR is populated)

Attribution

  • AI coding assistants used

… instead of xgrammar

Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com>
Assisted-by: CLAUDE:OPUS
Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com>
@github-actions github-actions Bot added the enhancement New feature or request label May 14, 2026
@github-actions
Copy link
Copy Markdown
Contributor

The PR description has been updated. Please fill out the template for your PR to be reviewed.

@jakelorocco jakelorocco marked this pull request as ready for review May 14, 2026 20:45
@jakelorocco jakelorocco requested review from a team as code owners May 14, 2026 20:45
Copy link
Copy Markdown
Contributor

@ajbozarth ajbozarth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On a quick read-through this looks good, Claude found a handful of small items, but nothing big:

Comment thread mellea/formatters/granite/base/util.py Outdated
Comment thread mellea/formatters/granite/base/util.py Outdated
Comment thread mellea/formatters/granite/base/util.py Outdated
Comment thread mellea/formatters/granite/base/util.py Outdated
Comment thread mellea/formatters/granite/base/util.py
Comment thread mellea/formatters/granite/base/util.py Outdated
Signed-off-by: Jake LoRocco <jake.lorocco@ibm.com>
Assisted-by: CLAUDE:OPUS
Comment on lines +20 to 27
from ....core.utils import MelleaLogger

if TYPE_CHECKING:
import llguidance
import torch
from transformers import PreTrainedModel, PreTrainedTokenizerBase

# First Party
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nite: import ordering

Suggested change
from ....core.utils import MelleaLogger
if TYPE_CHECKING:
import llguidance
import torch
from transformers import PreTrainedModel, PreTrainedTokenizerBase
# First Party
if TYPE_CHECKING:
import llguidance
import torch
from transformers import PreTrainedModel, PreTrainedTokenizerBase
# First Party
from ....core.utils import MelleaLogger

Copy link
Copy Markdown
Contributor

@ajbozarth ajbozarth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small nit, otherwise LGTM. Though it may be worth getting a pair of eyes that know the affected code a bit better to double check

@jakelorocco jakelorocco changed the title feat: consolidate to llguidance from xgrammar feat: consolidate to llguidance from xgrammar May 20, 2026
@akihikokuroda
Copy link
Copy Markdown
Member

There are some observations but any nothing major.

  1. Import style inconsistency (minor)
    - Line 131: import_optional is used inside the __call__ method but not at module level
    - Consider whether llguidance could be imported at module load time (if always available for granite formatters) or if lazy loading is intentional for performance
  2. Docstring incompleteness
    - _GuidanceLogitsProcessor.__init__ has a docstring but the class docstring says "Apply... to batch_scores" — it should clarify that it's a logits processor callback
    - Current: "A HuggingFace logits processor that enforces an llguidance grammar." (good enough, but could add a line about usage context)
  3. Error message ambiguity (line 305)
    - Old: Two separate error checks for missing tokenizer and model
    - New: One unified error mentioning both
    - The new error at line 333 requires both when constrained_decoding_prefix is used, but this is only enforced inside the nested conditional. Consider lifting this validation earlier or documenting
  the dependency more clearly.
  4. Type annotation edge case
    - Line 291: tokenizer: PreTrainedTokenizerBase | None = None but at line 311 it assumes tokenizer is not None without a guard when computing vocab_size. Guard added at line 312 (if ll_tokenizer is 
  None) mitigates this, but the logic flow could be clearer.
  5. Testing coverage not shown
    - Diff doesn't include test updates. PR says tests pass, but verify:
        - Unit tests for _GuidanceLogitsProcessor in the new location
      - Integration tests with ll_tokenizer parameter
      - Edge case: what happens if both tokenizer and ll_tokenizer are None and constrained decoding is requested?

Copy link
Copy Markdown
Member

@akihikokuroda akihikokuroda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request refactor

Projects

None yet

Development

Successfully merging this pull request may close these issues.

consolidate between llguidance and xgrammar fix: unpin xgrammar when bug is fixed

3 participants